Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

b4.4 bug: deprecate -t since it is gone in main, mixes poorly with -l, and exits when there is no preexisting log #1488

Merged
merged 1 commit into from
Nov 12, 2024
Merged

b4.4 bug: deprecate -t since it is gone in main, mixes poorly with -l, and exits when there is no preexisting log #1488

merged 1 commit into from
Nov 12, 2024
+2 −12

Conversation

baallan
Copy link
Collaborator

@baallan baallan commented Oct 29, 2024

truncating the log now works correctly whether the old log exists or not and independent of the ordering of -l and -t commands.

@baallan baallan mentioned this pull request Oct 29, 2024
Closed
@baallan
Copy link
Collaborator Author

baallan commented Oct 29, 2024

fixes #1443

@baallan baallan requested a review from nichamon October 29, 2024 21:24
@baallan baallan added this to the v4.4.5 milestone Oct 29, 2024
@baallan baallan changed the title fix -t option to work without dependence on order or log file preexistence b4.4 bug: fix -t option to work without dependence on order or log file preexistence Oct 29, 2024
nichamon
nichamon suggested changes Oct 30, 2024
View reviewed changes
Copy link
Collaborator

@nichamon nichamon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the change fixes the order dependency of -t and -l when they are specified at the command line, -t won't be supported when it is given in configuration files.

ldms/src/ldmsd/ldmsd.c Outdated Show resolved Hide resolved
@tom95858
Copy link
Collaborator

tom95858 commented Nov 1, 2024

@baallan, @nichamon this 'feature' has been removed from top of tree. Should we consider getting rid of this in 4.4? I know it's convenient to crush the log file at start up, but it can trivially be achieved in other ways with the new 'rm' command ;-)

@baallan
Copy link
Collaborator Author

baallan commented Nov 1, 2024 •
edited
Loading

@tom95858 If it is gone from main, I'm fine with making it a no-op+warning in 4.4. Just deleting it could break working scripts. But in theory this should be a community decision?

A patch to do that might also add deprecation warnings in 4.4 about all other options that are going away/gone in main.

@baallan
Copy link
Collaborator Author

baallan commented Nov 4, 2024

@tom95858 revised this pr to deprecate -t.

@baallan baallan changed the title b4.4 bug: fix -t option to work without dependence on order or log file preexistence b4.4 bug: deprecate -t since it is gone in main, mixes poorly with -l, and exits when there is no preexisting log Nov 4, 2024
@nichamon
Copy link
Collaborator

nichamon commented Nov 5, 2024

@baallan Thanks for making the revisions. Given that your latest commit removes the -t option completely, would you consider dropping the first commit dcc50b5?

@baallan
Copy link
Collaborator Author

baallan commented Nov 5, 2024

@nichamon if we could just check the squash box on the pr, that should take care of it. I don't have any pull/merge controls on the github repo, however.

@baallan
Deprecate -t option to ldmsd
b801e35 
This turns -t into a no-op and a warning.
@baallan
Copy link
Collaborator Author

baallan commented Nov 5, 2024

@nichamon found the branch and squashed it.

@tom95858 tom95858 merged commit 97c87ac into ovis-hpc:b4.4 Nov 12, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nichamon nichamon nichamon requested changes

Assignees

@tom95858 tom95858

Labels
None yet
Projects
None yet
Milestone
v4.4.5
Development

Successfully merging this pull request may close these issues.
3 participants
@baallan @tom95858 @nichamon